Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add maxInProgress option #650

Merged

Conversation

stephenplusplus
Copy link
Contributor

Continuing from #649

To Dos:

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jun 9, 2015
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@stephenplusplus
Copy link
Contributor Author

@leibale - as @googlebot says, please confirm you're okay with my incorporation of your work here :)

Also, please take a look when you can to make sure this works for you.

@stephenplusplus stephenplusplus changed the title Spp pubsub add max in progress add maxInProgress option Jun 9, 2015
@stephenplusplus stephenplusplus added the api: pubsub Issues related to the Pub/Sub API. label Jun 9, 2015
@leibale
Copy link
Contributor

leibale commented Jun 9, 2015

how confirm that? i already signed on CLA

@ryanseys
Copy link
Contributor

There's no formal way of telling the bot we are ok with it, it's not a very smart bot. :) Just confirm with us that you're cool with these changes.

@jgeewax
Copy link
Contributor

jgeewax commented Jun 11, 2015

I just double checked -- leibale signed the CLA.

@leibale
Copy link
Contributor

leibale commented Jun 11, 2015

@stephenplusplus my code working like a charm 😉.
my worker (in production) working with my fork repository few days.

@stephenplusplus
Copy link
Contributor Author

@leibale great :) I was hoping you could test this branch out as well, as I had to make some changes to your original work.

$ npm install --save stephenplusplus/gcloud-node#spp--pubsub-add-maxInProgress

@leibale
Copy link
Contributor

leibale commented Jul 1, 2015

what about that?

@stephenplusplus
Copy link
Contributor Author

The code in this PR is different than what you originally sent us. Did you try out the code from this PR?

@leibale
Copy link
Contributor

leibale commented Jul 1, 2015

in "stephenplusplus/gcloud-node#spp--pubsub-add-maxInProgress" branch no option to mark message as done without ack, i cant test..

@stephenplusplus
Copy link
Contributor Author

Can you explain that with some example code?

@leibale
Copy link
Contributor

leibale commented Jul 1, 2015

subscription.on('message', function (message) {
  request.get(message.data.url, function (err) {
    if (err) { // return the message to the queue
      return subscription.done(); // inProgress--
    }

    subscription.ack(message.ackId);
  });
});

@stephenplusplus
Copy link
Contributor Author

Okay, I think I misunderstood the idea originally, then. I think what you want is basically to be able to pause and resume the pulling? Would just adding .pause() and .resume() methods work for your use case?

@leibale
Copy link
Contributor

leibale commented Jul 1, 2015

no.. if i try to process message and i got error i need to down the inProgress without ack the message.

my english so bad :(

@leibale
Copy link
Contributor

leibale commented Jul 3, 2015

and..?

@stephenplusplus
Copy link
Contributor Author

Sorry @leibale, holiday weekend and #701 blew up my mind today 💣 👨 ☁️. I'll be revisiting this soon, and don't worry, the misunderstandings are my fault only.

@leibale
Copy link
Contributor

leibale commented Jul 10, 2015

i want to remove my fork and go back to master branch- time estimation for this enhancement?

@stephenplusplus
Copy link
Contributor Author

Sorry for the delays. I hopefully addressed this now with a new method, skip. So, from your previous example, this would look like:

subscription.on('message', function (message) {
  request.get(message.data.url, function (err) {
    if (err) {
      return message.skip();
    }

    subscription.ack(message.ackId); 
  });
});

Both skip and ack will -- the counter. Please try it out!

@leibale
Copy link
Contributor

leibale commented Jul 11, 2015

now it's looks good- i try it tomorrow

@leibale
Copy link
Contributor

leibale commented Jul 14, 2015

ack doesn't -- the counter (only skip)

@stephenplusplus
Copy link
Contributor Author

Woops, good catch. Thanks!

@leibale
Copy link
Contributor

leibale commented Jul 14, 2015

@stephenplusplus 👍
now i just want this on the master and.. :P

@stephenplusplus
Copy link
Contributor Author

Haha, me too! As long as you give me longer than an hour :) I hope to get a lot of the outstanding PRs merged by the end of the week, and if that goes well, I'll speed this one up.

@leibale
Copy link
Contributor

leibale commented Jul 14, 2015

ok. thanks alot for everything. :)

@leibale
Copy link
Contributor

leibale commented Jul 14, 2015

another mistake- you need to do the -- only if !err

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 15, 2015
@stephenplusplus
Copy link
Contributor Author

Thanks! Fixed.

@googlebot
Copy link

CLAs look good, thanks!

@stephenplusplus
Copy link
Contributor Author

@callmehiphop PTAL.

@callmehiphop
Copy link
Contributor

LGTM 👍

callmehiphop added a commit that referenced this pull request Jul 17, 2015
@callmehiphop callmehiphop merged commit c01e9fd into googleapis:master Jul 17, 2015
@stephenplusplus
Copy link
Contributor Author

Thanks for the review and thanks for the idea @leibale!

sofisl pushed a commit that referenced this pull request Oct 11, 2022
…andwritten libraries (#650)

- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 429395631

Source-Link: googleapis/googleapis@84594b3

Source-Link: googleapis/googleapis-gen@ed74f97
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZWQ3NGY5NzBmZDgyOTE0ODc0ZTZiMjdiMDQ3NjNjZmE2NmJhZmU5YiJ9
sofisl pushed a commit that referenced this pull request Oct 13, 2022
…andwritten libraries (#650)

- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 429395631

Source-Link: googleapis/googleapis@84594b3

Source-Link: googleapis/googleapis-gen@ed74f97
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZWQ3NGY5NzBmZDgyOTE0ODc0ZTZiMjdiMDQ3NjNjZmE2NmJhZmU5YiJ9
sofisl pushed a commit that referenced this pull request Nov 10, 2022
- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 470911839

Source-Link: googleapis/googleapis@3527566

Source-Link: googleapis/googleapis-gen@f16a1d2
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjE2YTFkMjI0ZjAwYTYzMGVhNDNkNmE5YTFhMzFmNTY2ZjQ1Y2RlYSJ9

feat: accept google-gax instance as a parameter
Please see the documentation of the client constructor for details.

PiperOrigin-RevId: 470332808

Source-Link: googleapis/googleapis@d4a2367

Source-Link: googleapis/googleapis-gen@e97a1ac
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZTk3YTFhYzIwNGVhZDRmZTczNDFmOTFlNzJkYjdjNmFjNjAxNjM0MSJ9
sofisl added a commit that referenced this pull request Nov 11, 2022
Co-authored-by: gcf-merge-on-green[bot] <60162190+gcf-merge-on-green[bot]@users.noreply.github.com>
Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>
sofisl pushed a commit that referenced this pull request Nov 11, 2022
…de 8 tests (#650)

* changes without context

        autosynth cannot find the source of changes triggered by earlier changes in this
        repository, or by version upgrades to tools such as linters.

* docs: more detailed docs on APIs such as Environment, Intents, Sessions.

PiperOrigin-RevId: 321243814

Source-Author: Google APIs <noreply@google.com>
Source-Date: Tue Jul 14 15:08:49 2020 -0700
Source-Repo: googleapis/googleapis
Source-Sha: 84ebe7c62f4c0875e001752fa84e87e629a1d418
Source-Link: googleapis/googleapis@84ebe7c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants